Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace BIND_VMETHOD by new GDVIRTUAL syntax #51970

Merged
merged 1 commit into from
Aug 22, 2021

Conversation

reduz
Copy link
Member

@reduz reduz commented Aug 22, 2021

  • New syntax is type safe.
  • New syntax allows for type safe virtuals in native extensions.
  • New syntax permits extremely fast calling.

Note: Everything was replaced where possible except for _gui_input _input and _unhandled_input.
These will require API rework on a separate PR as they work different than the rest of the functions.

Added a new method flag METHOD_FLAG_OBJECT_CORE, used internally. Allows to not dump the core virtuals like _notification to the json API, since each language will implement those as it is best fits.

@reduz reduz requested review from a team as code owners August 22, 2021 02:06
@reduz reduz force-pushed the implement-gdvirtuals-everywhere branch from ffd7bd4 to 91fd592 Compare August 22, 2021 02:39
@aaronfranke aaronfranke added this to the 4.0 milestone Aug 22, 2021
Copy link
Contributor

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks amazing! the missing ingredient I've wanted for so long for XRInterface, looking forward to applying it :)

@aaronfranke
Copy link
Member

scene/gui/control.cpp Outdated Show resolved Hide resolved
scene/gui/control.cpp Show resolved Hide resolved
core/extension/extension_api_dump.cpp Outdated Show resolved Hide resolved
core/io/resource.cpp Outdated Show resolved Hide resolved
} else {
color_map = _get_line_syntax_highlighting(p_line);
}
GDVIRTUAL_CALL(_get_line_syntax_highlighting, p_line, color_map);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks wrong, we're not calling _get_line_syntax_highlighting_impl, should be something like?

if (!GDVIRTUAL_CALL(_get_line_syntax_highlighting, p_line, color_map)) { 
    color_map = _get_line_syntax_highlighting_impl(p_line);
}

@@ -223,8 +222,6 @@ void ScriptEditorBase::_bind_methods() {
// TODO: This signal is no use for VisualScript.
ADD_SIGNAL(MethodInfo("search_in_files_requested", PropertyInfo(Variant::STRING, "text")));
ADD_SIGNAL(MethodInfo("replace_in_files_requested", PropertyInfo(Variant::STRING, "text")));

BIND_VMETHOD(MethodInfo("_add_syntax_highlighter", PropertyInfo(Variant::OBJECT, "highlighter")));
Copy link
Member

@Paulb23 Paulb23 Aug 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like an issue from #49546 and #49619, as it should be add_syntax_highlighter for the virtual add_syntax_highlighter method.

If I recall correctly, this method was only virtual bound so it would appear in the docs. as the main bound method is in the child classes, script_text_editor here, text_editor here and visual_script_editor here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could not figure out how this works, but it needs to be remade to work with the new system, if you can take a look and lend me a hand fixing it after I merge this PR I would be very grateful.

doc/classes/CodeEdit.xml Outdated Show resolved Hide resolved
@@ -81,9 +81,8 @@ Ref<EditorSyntaxHighlighter> EditorSyntaxHighlighter::_create() const {
void EditorSyntaxHighlighter::_bind_methods() {
ClassDB::bind_method(D_METHOD("_get_edited_resource"), &EditorSyntaxHighlighter::_get_edited_resource);

BIND_VMETHOD(MethodInfo(Variant::STRING, "_get_name"));
BIND_VMETHOD(MethodInfo(Variant::ARRAY, "_get_supported_languages"));
BIND_VMETHOD(MethodInfo(Variant::ARRAY, "_get_supported_extentions"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to rebind _get_supported_extentions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could not figure out how this works, as the way it was done did not work with the new system. The idea is to entirely get rid of BIND_VMETHOD (still need a few more PRs to achieve that) and only use the new system.

@reduz reduz force-pushed the implement-gdvirtuals-everywhere branch 2 times, most recently from e861517 to 9af9da1 Compare August 22, 2021 11:15
* New syntax is type safe.
* New syntax allows for type safe virtuals in native extensions.
* New syntax permits extremely fast calling.

Note: Everything was replaced where possible except for `_gui_input` `_input` and `_unhandled_input`.
These will require API rework on a separate PR as they work different than the rest of the functions.

Added a new method flag METHOD_FLAG_OBJECT_CORE, used internally. Allows to not dump the core virtuals like `_notification` to the json API, since each language will implement those as it is best fits.
@reduz reduz force-pushed the implement-gdvirtuals-everywhere branch from 9af9da1 to 3682978 Compare August 22, 2021 11:25
@reduz
Copy link
Member Author

reduz commented Aug 22, 2021

Ok, documentation should be properly kept now (used this PR as a test of #51982)

@reduz reduz merged commit a73b5fa into godotengine:master Aug 22, 2021
@neikeq
Copy link
Contributor

neikeq commented Aug 22, 2021

Added a new method flag METHOD_FLAG_OBJECT_CORE, used internally. Allows to not dump the core virtuals like _notification to the json API, since each language will implement those as it is best fits.

First time I hear this, I'm curious why a virtual like _notification would be implemented in a special way. Any example of such implementation?

@t3nk3y
Copy link

t3nk3y commented Sep 7, 2021

I wanted to point out that the following functions, which are intended to be overridden by plugins were renamed in this PR to keep with the spatial is now 3D naming. This would be a great thing to point out in an article detailing important changes devs need to be aware of when moving to Godot 4:

Godot 3

forward_spatial_gui_input
forward_spatial_draw_over_viewport
forward_spatial_force_draw_over_viewport

Godot 4

_forward_3d_gui_input
_forward_3d_draw_over_viewport
_forward_3d_force_draw_over_viewport

Also, the code only MOSTLY renames this, as the non-underscore functions within the base class have not been renamed. See below example where the function is named with "spatial" but calls the virtual "3d"

void EditorPlugin::forward_spatial_draw_over_viewport(Control *p_overlay) {
	GDVIRTUAL_CALL(_forward_3d_draw_over_viewport, p_overlay);
}

Mickeon added a commit to Mickeon/godot that referenced this pull request Sep 9, 2023
Reimplements the virtual method _setup_local_to_scene, lost in godotengine#51970

Also deprecates the redundant `setup_local_to_scene_requested` signal.
mandryskowski pushed a commit to mandryskowski/godot that referenced this pull request Oct 11, 2023
Reimplements the virtual method _setup_local_to_scene, lost in godotengine#51970

Also deprecates the redundant `setup_local_to_scene_requested` signal.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants